Computing full effective mappings for data stream mapping APIs #130498

masseyke · 2025-07-02T21:38:13Z

The update data stream mappings API (#130241) validates that the effective mapping after applying the user's mapping overrides is still a valid template. However, the effective mapping is generated using only the mapping overrides and the composable template's top-level mapping. It ignores any mappings provided by component templates within the composable template. This is a bug. This PR fixes that bug, and also uses the same new logic for generating the effective_mappings that are returned to the user in the get and update data stream mappings APIs.

elasticsearchmachine · 2025-07-07T12:59:35Z

Pinging @elastic/es-data-management (Team:Data Management)

Copilot

Pull Request Overview

This PR fixes a bug where data stream effective mappings did not include component templates, and unifies mapping computation logic across the get and update data stream mappings APIs.

Introduces a new getEffectiveMappings overload that merges composable and component template mappings via IndicesService.
Updates services and transport actions to pass IndicesService into mapping computation.
Exposes helper methods in ComposableIndexTemplate and adjusts tests and YAML fixtures to validate the merged mappings.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
server/src/main/java/.../DataStream.java	Adds overloaded `getEffectiveMappings` with full merge logic.
server/src/main/java/.../MetadataDataStreamsService.java	Switches to new mapping logic for update APIs.
server/src/main/java/.../MetadataCreateIndexService.java	Refines `collectV2Mappings` exception signatures.
server/src/main/java/.../ComposableIndexTemplate.java	Makes merge and conversion helpers public.
modules/data-streams/.../TransportUpdateDataStreamMappingsAction.java	Injects `IndicesService` and uses new API.
modules/data-streams/.../TransportGetDataStreamMappingsAction.java	Injects `IndicesService` and uses new API.
server/.../DataStreamTests.java	Updates test to call new `getEffectiveMappings` signature.
modules/data-streams/.../250_data_stream_mappings.yml	Adjusts YAML expectations for merged mappings.
modules/data-streams/.../TransportUpdateDataStreamMappingsActionIT.java	Updates IT to include timestamp and component fields.
modules/data-streams/.../DataStreamIT.java	Adds end-to-end test for new effective mappings logic.

Comments suppressed due to low confidence (3)

modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java:2555

[nitpick] Instantiating a new ActionType here bypasses the existing UpdateDataStreamMappingsAction.INSTANCE. For consistency and to avoid duplicate registrations, use the provided INSTANCE constant.

            new ActionType<UpdateDataStreamMappingsAction.Response>(UpdateDataStreamMappingsAction.NAME),

server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java:456

The call to collectV2Mappings uses five arguments, but no such overload exists in MetadataCreateIndexService. This will fail to compile. Consider invoking the correct helper for merging component mappings, or add a matching overload.

        final List<CompressedXContent> mappings = collectV2Mappings(

server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamTests.java:2692

The test uses mock(...) but doesn't import Mockito’s mock method. Add import static org.mockito.Mockito.mock; (or a qualified Mockito.mock) to avoid compilation errors.

        IndicesService indicesService = mock(IndicesService.class);

lukewhiting

LGTM from a code cleanliness perspective but I would be more comfortable if this gets a second thumb from someone more familiar with templates and rollover

dakrone

LGTM, I left a couple of questions.

dakrone · 2025-07-08T15:09:56Z

server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java

+            Map<String, Object> originalMappingMap = XContentHelper.convertToMap(
+                documentMapper.mappingSource().uncompressed(),
+                true,
+                XContentType.JSON
+            ).v2();
+            if (originalMappingMap.containsKey(MapperService.SINGLE_MAPPING_NAME)) {
+                resultMapping = (Map<String, ?>) originalMappingMap.get(MapperService.SINGLE_MAPPING_NAME);
+            } else {
+                resultMapping = originalMappingMap;
+            }
+            return convertMappingMapToXContent(resultMapping);


Seems a little strange that we uncompress the xcontent, convert it to a map, potentially strip the mapping name, then reconvert it to a compressed xcontent. Should we instead leave the mapping name so that we don't have to do the extra conversions?

I was just trying to avoid sometimes having _doc in the effective mappings for the sake of consistency. I can remove this part if you think it's overkill.

When we do the check, does the default (let's say "undeprecated") version match the originalMappingMap.containsKey(MapperService.SINGLE_MAPPING_NAME) condition?

I don't understand the question. The default is _doc, which is MapperService.SINGLE_MAPPING_NAME, right?

Sorry, what I meant is, in the current world, is the default to have _doc in the mapping, or is it elided by default?

I believe we usually just deal with it if it exists. It seems confusing as a user (given that most users are probably unaware that we used to allow multiples since that has been gone for quite a while). I had just been trying to make it simpler for users of a new API so that they didn't have to write client code that can handle the result two different ways.

dakrone · 2025-07-08T15:13:29Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsService.java

-            dataStream.getEffectiveMappings(projectMetadata),
-            indicesService
-        );
+        MetadataIndexTemplateService.validateTemplate(mergedEffectiveSettings, null, indicesService);


Can you elaborate on why we don't want to use the effective mappings for validation here? We don't currently, but in the future we could have settings that affect mappings, so we may still want to validate for that here.

My thinking was that it didn't affect any of the settings we're allowing, so why do the now-more-expensive operation to get effective mappings? I can put it back though -- the mapping-merging wound up being faster than I expected.

I think it'd be best to put it back just so we don't accidentally miss it some time in the future.

dakrone · 2025-07-08T15:14:37Z

server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamTests.java

+        IndicesService indicesService = mock(IndicesService.class);
+        assertThrows(IllegalArgumentException.class, () -> dataStream.getEffectiveMappings(projectMetadataBuilder.build(), indicesService));


Do we need the mock, or can we just pass null in for the service here?

Oh you're right. I'll change that.

…ic#130498)

Computing full effective mappings for data stream mapping APIs

1f76e32

masseyke added >non-issue :Data Management/Data streams Data streams and their lifecycles v9.2.0 labels Jul 2, 2025

masseyke added 5 commits July 3, 2025 12:34

fixing TransportUpdateDataStreamMappingsActionIT

7415ce6

merging main

2a6474a

cleanup

b565a53

adding a test for getEffectiveMappings

25c36db

Merge branch 'main' into data-stream-mappings-effective-mappings

e8f7527

masseyke requested a review from Copilot July 3, 2025 21:32

This comment was marked as outdated.

Sign in to view

masseyke added 4 commits July 3, 2025 16:42

using a more specfic exception

a96ad86

Merge branch 'main' into data-stream-mappings-effective-mappings

e840204

using a more specfic exception

b055edc

using a more specfic exception

cdd1ef8

masseyke marked this pull request as ready for review July 7, 2025 12:59

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Jul 7, 2025

masseyke requested review from dakrone and lukewhiting July 7, 2025 12:59

lukewhiting requested a review from Copilot July 7, 2025 14:46

Copilot AI reviewed Jul 7, 2025

View reviewed changes

lukewhiting approved these changes Jul 7, 2025

View reviewed changes

dakrone approved these changes Jul 8, 2025

View reviewed changes

masseyke added 6 commits July 8, 2025 12:13

removing unnecessary mock

bf3d22f

using effective mappings for settings validation

8264a38

getting rid of code to remove _doc from effective mappings

3393acc

Merge branch 'main' into data-stream-mappings-effective-mappings

7a11b6a

getting rid of code to remove _doc from effective mappings

b1daeea

getting rid of code to remove _doc from effective mappings

ed07473

masseyke merged commit 1868594 into elastic:main Jul 9, 2025
33 checks passed

masseyke deleted the data-stream-mappings-effective-mappings branch July 9, 2025 21:13

mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jul 17, 2025

Computing full effective mappings for data stream mapping APIs (elast…

9c6703b

…ic#130498)

mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jul 17, 2025

Computing full effective mappings for data stream mapping APIs (elast…

dc15017

…ic#130498)

		IndicesService indicesService = mock(IndicesService.class);
		assertThrows(IllegalArgumentException.class, () -> dataStream.getEffectiveMappings(projectMetadataBuilder.build(), indicesService));

Computing full effective mappings for data stream mapping APIs #130498

Computing full effective mappings for data stream mapping APIs #130498

Uh oh!

Conversation

masseyke commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

elasticsearchmachine commented Jul 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

lukewhiting left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

masseyke commented Jul 2, 2025 •

edited

Loading

dakrone Jul 8, 2025 •

edited

Loading